Fix convention of PolynomialTensor basis change#805
Conversation
In general, we rotate a matrix M to M' is to apply a rotation matrix R:
M' = R @ M @ R.T
The corresponding Einstein notation is
M'^{p_1p_2} = R^{p_1}_{a_1} R^{p_2}_{b_1} M^{a_1a_2}
|
@arettig Would you be able to take a look at this and check the technical correctness? (No rush.) |
arettig
left a comment
arettig
left a comment
There was a problem hiding this comment.
This is a good catch. The PolynomialTensor rotation code currently does not match the expression in the documentation. We should change this to match standard convention (and our own documentation), but this is a potentially breaking change for anyone using this function currently.
It would also be a good idea to change the 90 degree rotation test added in this PR to some different rotation as this test actually passes with the current (incorrect) implementation.
| polynomial_tensor.rotate_basis(rotation_matrix_reverse) | ||
| self.assertEqual(polynomial_tensor, want_polynomial_tensor) | ||
|
|
||
| def test_rotate_basis_90deg(self): |
There was a problem hiding this comment.
This test actually does not catch the bug you pointed out since for this specific case
The 90 degree rotation test passed even with previous erroneous code. This new cyclical rotation (0->1, 1->2, 2->0) test will correctly test the fixed bug.
4 participants
In general, we rotate a matrix M to M' is to apply a rotation matrix R: M' = R @ M @ R.T
The corresponding Einstein notation is
M'^{p_1p_2} = R^{p_1}{a_1} R^{p_2}{b_1} M^{a_1a_2}